-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build datastore index resource. #3085
Build datastore index resource. #3085
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 26 files changed, 763 insertions(+), 47 deletions(-)) |
Haven't written examples yet, sending out for early review to make sure I'm headed down the right road. Works in testing on my machine! |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 26 files changed, 763 insertions(+), 47 deletions(-)) |
599fccb
to
c180935
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 26 files changed, 996 insertions(+), 40 deletions(-)) |
c180935
to
09ef2e2
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 26 files changed, 996 insertions(+), 40 deletions(-)) |
09ef2e2
to
191194a
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 1108 insertions(+), 40 deletions(-)) |
@danawillow Okay, seems to be working for me, ready for review! |
@@ -234,8 +234,39 @@ func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{ | |||
} | |||
<% end -%> | |||
<% if object.async.is_a? Api::OpAsync -%> | |||
<% if object.async.result.resource_inside_response and not object.identity.empty? -%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check- is the index id not known until the operation is finished, or is it also contained in the first operation that we then poll? I know firestore_index currently solves this with custom code in the post create, because the name is contained in the first operation response that we already have. Looking at the generated output for this we now set the id on that resource 3 times in create, which isn't ideal. Is there a clean solution that would work for that resource and this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right - it's not known until the operation is finished. :( I saw that it could be solved with custom code, but I wanted to make sure that I solved it Once And For All - is there an issue with setting the ID three times? I think we set it once to make sure that we'll do a refresh if we fail in the middle of polling, plus once to get the true final id once all information is known ... and then in firestore, once more due to the custom code solution. I agree it's not clean, and I'm struggling to test it so I don't even know if it works yet (since you can't use the same project for both firestore and datastore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I'm pretty ok with it, then. It shouldn't cause issues for users and especially with these templates going away at some point, I don't think it's worth spending a ton of time finding the most-clean-code way to solve this problem for two resources with similar but not identical needs.
Resolved all comments but one - the uncleanliness of this solution when combined with Firestore's solution for a similar, but different, problem. I can try to find something, but at first glance it's not obvious how to avoid it, other than making this operation change into custom code. I resolve that tradeoff in this direction, but I'm willing to change it - is it worth it, in your opinion? |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 26 files changed, 994 insertions(+), 40 deletions(-)) |
babb8c7
to
03ee74d
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 1003 insertions(+), 40 deletions(-)) |
Add datastore/ product Add ability for fetching resources from operations
… port old code over to.
03ee74d
to
ef337a3
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 1003 insertions(+), 40 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 1012 insertions(+), 40 deletions(-)) |
be5fabb
to
11cc3b6
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 1012 insertions(+), 40 deletions(-)) |
Add datastore/ product
Add ability for fetching resources from operations
Fixes hashicorp/terraform-provider-google#60
Release Note Template for Downstream PRs (will be copied)